Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add checksum validation to KCL packages #243

Merged
merged 7 commits into from
Nov 21, 2024

Conversation

NishantBansal2003
Copy link
Contributor

1. Please confirm that you have read the document before PR submitted

2. Contact Information(Optional)

If it is convenient, please provide your contact information so we can reach you when processing the PR:

@NishantBansal2003
Copy link
Contributor Author

NishantBansal2003 commented Oct 12, 2024

Hey @zong-zhe, I want to write a test for the Go tool I developed that includes a checksum. I plan to first push a test package to a local registry using Docker and then validate my code by updating the sum field in the manifest. However, when writing the test, I'm using "kcl-lang.io/kpm/pkg/mock", which results in an error since it hasn't been released in KPM's latest version. How can I now use the KPM binary to push a KCL package to the local registry and test my code?
kcl-lang/kpm#431 (comment)
Edit: Currently, I am creating another mock in the modules repository to generate the KPM binary and push a test package to the local registry. To do this, I am using the following command to generate the KPM binary: GOBIN=$(pwd)/bin go install kcl-lang.io/cli/cmd/kcl@latest, Is this approach okay?

@NishantBansal2003
Copy link
Contributor Author

This PR is ready for review. Feedback and suggestions for improvement are appreciated.
cc: @zong-zhe

@NishantBansal2003
Copy link
Contributor Author

Through this code, a new manifest (with a checksum field) and old blobs (config, layers) will be generated and tagged with the corresponding version of the KCL package. However, the old manifest will remain in the registry as a dangling manifest without a tag.

eminaktas added a commit to eminaktas/modules that referenced this pull request Oct 19, 2024
@NishantBansal2003 NishantBansal2003 changed the title [WIP]: Added functionality to include checksum in kcl packages feat: add checksum validation to KCL packages Oct 22, 2024
@Peefy
Copy link
Contributor

Peefy commented Nov 4, 2024

cc @zong-zhe

@Peefy
Copy link
Contributor

Peefy commented Nov 8, 2024

cc @zong-zhe

Signed-off-by: Nishant Bansal <[email protected]>
@NishantBansal2003
Copy link
Contributor Author

@zong-zhe PTAL

@NishantBansal2003
Copy link
Contributor Author

Hey @zong-zhe
I am confirming how much work is left for the LFX part. After merging the PR that includes the checksum and integrating the checksum checker in KPM, will these two PRs remain, or are there any more left for the LFX? I want to clarify this so I can divide the tasks and work on them accordingly.

@Peefy
Copy link
Contributor

Peefy commented Nov 18, 2024

cc @zong-zhe

@NishantBansal2003
Copy link
Contributor Author

NishantBansal2003 commented Nov 20, 2024

Any comments on this? Please let me know as the deadlines are approaching quickly.
Also, I need a bit of clarification on the next steps after this, since we can't proceed immediately with the checksum integration PR (kcl-lang/kpm#520) as it requires all the packages to include the checksum field.
@Peefy @zong-zhe

Integrate-Checksum/main.go Show resolved Hide resolved
Integrate-Checksum/main.go Show resolved Hide resolved
Integrate-Checksum/main.go Show resolved Hide resolved
Integrate-Checksum/main.go Outdated Show resolved Hide resolved
Integrate-Checksum/main.go Show resolved Hide resolved
Signed-off-by: Nishant Bansal <[email protected]>
@NishantBansal2003
Copy link
Contributor Author

Done, ptal
cc: @zong-zhe

Integrate-Checksum/main.go Outdated Show resolved Hide resolved
Integrate-Checksum/main.go Outdated Show resolved Hide resolved
Integrate-Checksum/main.go Outdated Show resolved Hide resolved
Integrate-Checksum/main.go Outdated Show resolved Hide resolved
Signed-off-by: Nishant Bansal <[email protected]>
@NishantBansal2003
Copy link
Contributor Author

Any comments on this? Please let me know as the deadlines are approaching quickly. Also, I need a bit of clarification on the next steps after this, since we can't proceed immediately with the checksum integration PR (kcl-lang/kpm#520) as it requires all the packages to include the checksum field. @Peefy @zong-zhe

Also could you please clarify this?

@zong-zhe
Copy link
Collaborator

Any comments on this? Please let me know as the deadlines are approaching quickly. Also, I need a bit of clarification on the next steps after this, since we can't proceed immediately with the checksum integration PR (kcl-lang/kpm#520) as it requires all the packages to include the checksum field. @Peefy @zong-zhe

Also could you please clarify this?

😄 I will update it later in this PR kcl-lang/kpm#520

Copy link
Collaborator

@zong-zhe zong-zhe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zong-zhe zong-zhe merged commit 499275d into kcl-lang:main Nov 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants